-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding global error handler for ajax calls which run into redirection… #16783
Conversation
A new inspection was created. |
This fix is fine for testing, but note that it might break in cases where 302 might be an expected response (not sure which ones though). There is a small chance that this breaks WebDAV connectivity detection as it might rely on 401 error codes. Not tested yet, but something to look into if we decide to keep this fix after all. Ideal would be to have the server always return 401 instead of anything else when the authentication expired. |
302 is used with shibboleth to redirect to the idp in case of session expiry - we might want to add some additional shib specific criteria .... |
Is that something that sits between Apache and the PHP code ? (aka hard to detect) |
well the js code might extract some information from the response itself - maybe some header used in an shib env .... |
I tested this and ran into:
As a result the status is 0 not 30x. Solved that by changing: - if (_.contains([302, 307, 401], request.status)) {
+ if (_.contains([0, 302, 307, 401], request.status)) { After that the redirect worked bautifully for the list request, share action and heartbeat. @DeepDiver1975 @LukasReschke does that make sense? |
@LukasReschke @DeepDiver1975 should I just add that change as a commit? Then we test webdav again? |
Without shib enabled we should only react on 401. All the other cases are shibboleth only and should be treated within the shibboleth apps. |
How does shib behave with Webdav endpoints ? (asking for #16902) |
Defer to 9.0 ? |
0bbef64
to
e59b6a8
Compare
If one of several webservers is down, we would need to handle a 503. Is it missing? I am not sure ... |
Well if a webserver is physically down there should be no response at all and a timeout is raised. |
That should work now. Appframework does not need any adjustments from what I can tell and tested. 😄 |
Nice, thanks a lot @LukasReschke! |
Works! @MorrisJobke if you like could you retry your test case ? |
Really nice - works now in all my tested scenarios like:
👍 Another topic is the upload handling. There it is already covered but only prompts a "Authentication error" and doesn't redirects. I'm open to do this in a new PR, because it's another code path or to directly put this in here. It's your decision. |
@MorrisJobke @PVince81 That should be done with b99c6f1 as well now. That's |
@LukasReschke does that 401 also sends the DummyBasicAuth ? |
No. Because it won't reply with any auth request at all. It will simply return a 401 with a JSON blob stating that the auth failed. |
Ok, so let's test again with IE9 and IE10 to make sure it still works, then merge this. This 👍 is for @LukasReschke's code. Will need another thumbs up for the JS code. |
Needs a second reviewer for the JS changes @LukasReschke @nickvergessen @rullzer |
Note, redirecting would cause whatever input the user had to be lost. This is fine short term, raised #22458 to discuss having a popup login dialog instead. |
👍 for the JS changes. Short-term this seems like the way to go. Better than before 😄 |
Adding global error handler for ajax calls which run into redirection…
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
…s or unauthorized responses
refs #16726
@butonic @MorrisJobke
Let see how far we re getting with this one.